feat(nodemetadataenricher): Add log signal support#2095
Open
miconeilaws wants to merge 4 commits into
Open
Conversation
Contributor
|
This PR was marked stale due to lack of activity. |
louisall
force-pushed
the
miconeil/nodemetadataenricher-log-support
branch
2 times, most recently
from
9601128 to
3883567
Compare
sky333999
previously approved these changes
May 19, 2026
louisall
force-pushed
the
miconeil/nodemetadataenricher-log-support
branch
from
3883567 to
ad561a8
Compare
louisall
added
the
ready for testing
sky333999
reviewed
May 19, 2026
louisall
added a commit
that referenced
this pull request
May 20, 2026
Mirror the existing metrics tests on the logs path so processLogs has direct unit coverage. Adds a createTestLogs helper and 7 mirror tests for cache hit, cache miss, missing/empty node name, log count preservation, AZ overwrite, and mixed input handling. Also adds TestLazyInitLogsCache, which exercises the only branch in processLogs whose behavior diverges across calls — cache nil at construction, populated before first invocation. Verifies the processor lazy-loads the cache, stores the reference, and continues to enrich on subsequent calls even after the singleton is cleared. Addresses sky333999's review feedback on PR #2095.
Collaborator
|
4 failing checks are unrelated: sles-15:ca_bundle_test fails on both amd64 and arm64 (showing as 2 reds). The matrix hardcodes caCertPath: /etc/ssl/certs/ca-bundle.crt, which doesn't exist on SLES 15 — SUSE puts it at /etc/ssl/ca-bundle.pem. Fails on every PR rocky-linux-9:ssm_document_test is a 5-minute SSM-ready timeout — the EC2 instance didn't register with Systems Manager in time. Same flake has hit several recent unrelated PRs. Verify All PR Test Jobs is the workflow aggregator and just rolls up the 3 above. |
louisall
added a commit
that referenced
this pull request
May 27, 2026
Mirror the existing metrics tests on the logs path so processLogs has direct unit coverage. Adds a createTestLogs helper and 7 mirror tests for cache hit, cache miss, missing/empty node name, log count preservation, AZ overwrite, and mixed input handling. Also adds TestLazyInitLogsCache, which exercises the only branch in processLogs whose behavior diverges across calls — cache nil at construction, populated before first invocation. Verifies the processor lazy-loads the cache, stores the reference, and continues to enrich on subsequent calls even after the singleton is cleared. Addresses sky333999's review feedback on PR #2095.
louisall
force-pushed
the
miconeil/nodemetadataenricher-log-support
branch
from
eff009b to
13fa009
Compare
Extend the nodemetadataenricher processor to handle logs in addition to metrics. The processLogs method uses the same logic as processMetrics: looks up k8s.node.name in resource attributes and stamps host.id, host.name, host.type, host.image.id, and cloud.availability_zone from the Lease-based node metadata cache. This enables the cluster-scraper's K8s events pipeline to enrich event logs with per-node IMDS metadata.
Mirror the existing metrics tests on the logs path so processLogs has direct unit coverage. Adds a createTestLogs helper and 7 mirror tests for cache hit, cache miss, missing/empty node name, log count preservation, AZ overwrite, and mixed input handling. Also adds TestLazyInitLogsCache, which exercises the only branch in processLogs whose behavior diverges across calls — cache nil at construction, populated before first invocation. Verifies the processor lazy-loads the cache, stores the reference, and continues to enrich on subsequent calls even after the singleton is cleared. Addresses sky333999's review feedback on PR #2095.
louisall
force-pushed
the
miconeil/nodemetadataenricher-log-support
branch
from
13fa009 to
13c8e83
Compare
movence
previously approved these changes
Jun 4, 2026
mitali-salvi
reviewed
Jun 4, 2026
| ) | ||
| } | ||
|
|
||
| func createLogsProcessor( |
Contributor
There was a problem hiding this comment.
missing unit tests for this function and maybe this file as well
| return md, nil | ||
| } | ||
|
|
||
| func (p *nodeMetadataEnricherProcessor) processLogs(_ context.Context, ld plog.Logs) (plog.Logs, error) { |
Contributor
There was a problem hiding this comment.
seems like this is an almost duplicate of processMetrics, can we extract the common functionality into a util function and use it inside both processing functions ?
… add factory tests processMetrics and processLogs were near-byte-identical, with the same lazy-init prelude and the same per-resource enrichment loop body. Extract both into helpers: - resolveCache() — handles the cache.Load + lazy-init pattern, returning nil when the extension is still unavailable. - enrichResource(pcommon.Resource, *NodeMetadataCache) — performs the node-name lookup and writes the 5 host attributes. Operates on the signal-agnostic pcommon.Resource type so both pipelines reuse it. processMetrics and processLogs become ~6 lines each — cache resolve + slice walk. Public API unchanged; all 17 existing unit tests pass through the refactored code path. Also adds factory_test.go covering NewFactory, createDefaultConfig, and both createMetricsProcessor and createLogsProcessor (including their configuration-type assertion error branches). Addresses mitali-salvi's review feedback on PR #2095.
Contributor
|
This PR was marked stale due to lack of activity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend the nodemetadataenricher processor to handle logs in addition to metrics. The processLogs method uses the same logic as processMetrics: looks up k8s.node.name in resource attributes and stamps host.id, host.name, host.type, host.image.id, and cloud.availability_zone from the Lease-based node metadata cache.
This enables the cluster-scraper's K8s events pipeline to enrich event logs with per-node IMDS metadata.